Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

First solution, basic tasks solved #1014

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

noiseintheattic
Copy link

No description provided.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work on implementing the features and adhering to the checklist requirements! 👍 However, there are a few areas that need some attention to ensure the code is robust and maintainable. Specifically, consider handling potential exceptions more gracefully when parsing product information, optimizing instance creation by checking the map first, and refining exception messages for better clarity. Keep up the good work, and with these adjustments, your code will be even stronger! 💪

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

src/main/java/mate.academy/lib/Injector.java Outdated Show resolved Hide resolved
src/main/java/mate.academy/lib/Injector.java Outdated Show resolved Hide resolved
src/main/java/mate.academy/lib/Injector.java Outdated Show resolved Hide resolved
src/main/java/mate.academy/lib/Injector.java Outdated Show resolved Hide resolved
Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey there! 👋 It looks like you've done a great job overall, but there are a few critical issues that need addressing before we can merge your Pull Request. Specifically, there are some potential bugs related to object instantiation and exception handling that could affect the functionality of your code. Also, don't forget to add the @Target annotation to your custom annotations for clarity on their usage. Keep up the good work, and once these issues are resolved, your code will be in great shape! 🚀

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

src/main/java/mate.academy/Main.java Show resolved Hide resolved
src/main/java/mate.academy/Main.java Show resolved Hide resolved
src/main/java/mate.academy/lib/Component.java Show resolved Hide resolved
src/main/java/mate.academy/lib/Injector.java Show resolved Hide resolved
src/main/java/mate.academy/lib/Injector.java Show resolved Hide resolved
src/main/java/mate.academy/lib/Injector.java Show resolved Hide resolved
src/main/java/mate.academy/lib/Injector.java Outdated Show resolved Hide resolved
src/main/java/mate.academy/lib/Injector.java Outdated Show resolved Hide resolved
…target annotation, changed order in getInstatnce() method, gathered exceptions in ReflectiveOperationException
Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work on your Pull Request! 🎉 Your use of annotations and dependency injection is on point. Remember to include unit tests for singleton functionality, as they are essential for verifying your code's correctness outside of the main application flow. Keep up the good work, and always aim to refine and improve your code! 👍

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

Comment on lines 11 to +19
// Please test your Injector here. Feel free to push this class as a part of your solution
Injector injector = Injector.getInjector();
ProductService productService = null;
ProductService productService = (ProductService) injector.getInstance(ProductService.class);

// Checking the singleton pattern (on AI mentor request)
Injector injectorSecond = Injector.getInjector();
if (!injector.equals(injectorSecond)) {
throw new RuntimeException("The singleton pattern doesn't work correctly");
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not necessary to test the singleton pattern functionality within the main method of the application. This should be covered by dedicated unit tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants